fix(e2e): Ensures Maven proxy test is not infra dependant #5581
Conversation
There was a problem hiding this comment.
LGTM. However I'd try to make an effort to remove platform specific conditions at all when we find them. We are already suffering this legacy situation and have difficulty to maintain the whole codebase. IMO, the negative test could be enough to prove that, when using a non existing proxy, the operator cannot build, therefore we may get rid off the "CONNECT" check condition.
|
|
||
| if !ocp { | ||
| // OpenShift has internal configuration (like oauth) that overriddes the default repo connect URL | ||
| g.Expect(logs).To(ContainSubstring("\"CONNECT repo.maven.apache.org:443 HTTP/1.1\" 200")) |
There was a problem hiding this comment.
I'd avoid filling our code (also test code) with platform specific conditions. I think we need to find a general way to make a test.
There was a problem hiding this comment.
I removed the check but kept the passing test. I think proof of build with the proxy present goes by hand with the build error without the proxy. The proxy being test code driven makes it less infra dependant.
But if you think it is too much to maintain I can remove it as well.
There was a problem hiding this comment.
I think that's fine. What we should understand is if there is anything specific in the operator log that can make us check that we're connecting to a proxy. However, this could be something done at OS level, so, I think the reasoning on removing the check is fine by assuming that any third party will do its work. In theory, we should be fine by setting the env var and any tooling should be honoring such a configuration. However, Maven is a bit particular, and it requires an additional setting [1]. This is in fact something we are already doing, see:
camel-k/pkg/util/maven/maven_proxies.go
Line 30 in cb74737
A follow up work we can do (into this or after this PR) is to add some unit test to validate that, in presence of the HTTP_PROXY vars, the settings are created as expected. And the E2E can validate the presence of such configuration in the maven settings created next to the IntegrationPlatform.
There was a problem hiding this comment.
Good ideas.
Let's merge this PR as it is and I will be adding some UT and improve the e2e configuration check in another PR.
gansheer
force-pushed
the
fix/maven_http_proxy_test
branch
from
78e9acb
to
c0f9275
Compare
gansheer
force-pushed
the
fix/maven_http_proxy_test
branch
from
c0f9275
to
f189da0
Compare
Modifications on maven http proxy test modification to avoid infra configuration dependance:
Both test should be enought to prove the maven proxy test works.
Release Note